Fix Glint types by converting helpers to plain functions #188
Conversation
|
@NullVoxPopuli thanks for submitting this! Do you see this solve any TS issues we tried to solve and couldn't in #185? |
NullVoxPopuli
force-pushed
the
copy-pr-185
branch
from
2bf741a to
d560c7b
Compare
|
perhaps! |
NullVoxPopuli
force-pushed
the
copy-pr-185
branch
from
7f07ca1 to
52e9a16
Compare
| for (let i = 0, len = params.length; i < len; i++) { | ||
| if (truthConvert(params[i]) === false) { | ||
| return params[i] as boolean; | ||
| } | ||
| } | ||
| return params[params.length - 1] as boolean; |
There was a problem hiding this comment.
keeping the as boolean's here might not quite meet the type needs of #185 for the and helper
There was a problem hiding this comment.
(not that this PR needs to solve that too)
| @@ -16,7 +16,7 @@ | |||
| "scripts": { | |||
| "build": "concurrently 'npm:build:*'", | |||
| "build:js": "rollup --config", | |||
| "build:types": "glint --declaration && grep -r -l '///' declarations/ | sort | uniq | xargs perl -e \"s/\\/\\/\\/.*$//\" -pi", | |||
There was a problem hiding this comment.
this is fixed
|
@NullVoxPopuli and other who follow: I tried to bump this in our app (via https://github.com/jmurphyau/ember-truth-helpers/tree/helpers-as-plain-functions-dist) and see many of tests fail, will look into root cause why behavior is different. Glint and TS were happy though. |
do you have a reproduction? |
sorry, it's behind VPN. I'm digging what exactly is the issue and if it's our app code or the lib code issue |
| } | ||
|
|
||
| export default helper<AndSignature>((params) => { | ||
| export default function and<T extends MaybeTruth[]>(...params: [...T]) { |
There was a problem hiding this comment.
@NullVoxPopuli using spread causes an issue - today, arguments gets lazily evaluated and spread makes it process eager.
e.g. imagine use case
today, if (foo "A") returns false, then (bar "B") does not get evaluated.
but this line change makes it evaluated which is definitely not what we want
There was a problem hiding this comment.
that's just how plain functions work though: https://github.com/glimmerjs/glimmer-vm/blob/68d371bdccb41bc239b8f70d832e956ce6c349d8/packages/%40glimmer/manager/lib/internal/defaults.ts#L38
There was a problem hiding this comment.
to make or lazy or short-circuit, it needs to be built in to the VM.
There was a problem hiding this comment.
I wonder if or and and need to remain "classic" I wonder if that means converting to class helpers so that we can correctly use generics?
| } | ||
|
|
||
| export default helper<OrSignature>((params) => { | ||
| export default function or<T extends MaybeTruth[]>(...params: [...T]) { |
There was a problem hiding this comment.
same issue here as in and helper
that way, we preserve semantic of them lazily evaluating arguments and able to accept generics
SergeAstapov
changed the title
SergeAstapov
force-pushed
the
copy-pr-185
branch
from
3de805c to
7b251ea
Compare
|
Made a tool to help out in the future: https://github.com/NullVoxPopuli/fix-bad-declaration-output |
This is a breaking change without the polyfill, and would require [email protected] without said polyfill.
This conversion to normal functions cooincidentally fixes the triple slash issue, described